feat: configure Enterprise SSO for Replicated VM deployments - #1116

Reviewed against the OpenHands-Cloud Helm chart guidelines. The Keycloak auto-provisioning logic itself is solid, but there's an important configuration-naming problem that will bite self-hosted consumers.

Important - two near-identical values keys (enterpriseSso vs enterpriseSSO)

This PR adds a new key enterpriseSso (lowercase sso) at charts/openhands/values.yaml:117, but the chart already ships enterpriseSSO (capital SSO) at charts/openhands/values.yaml:224 (added in #325). YAML keys are case-sensitive, so these are two independent toggles that differ only by capitalization:

• enterpriseSSO.enabled (existing) -> templates/_env.yaml uses .Values.enterpriseSSO.enabled to add "enterprise_sso" to OH_WEB_CLIENT_PROVIDERS_CONFIGURED.
• enterpriseSso.enabled + idpMetadataUrl (new) -> gates the Keycloak SAML IdP auto-provisioning in keycloak-config-script.yaml.

Having enterpriseSso and enterpriseSSO side by side in the same values.yaml is a serious foot-gun: a consumer who sets one will almost certainly assume they set the other. This runs against the "consumer-friendly configuration / one knob, one effect" principle. I'd strongly suggest consolidating onto the existing enterpriseSSO key (e.g. add displayName and idpMetadataUrl under it) so a single toggle drives both the web-client provider advertisement and the Keycloak auto-config.

Important - consumer split-brain for raw-Helm installs

Because of the two keys, enabling SSO end-to-end via the raw chart currently requires setting both enterpriseSSO.enabled: true (so the provider is advertised to the web client) and enterpriseSso.enabled: true + idpMetadataUrl (so Keycloak is configured). The new README.md section only documents the lowercase enterpriseSso key, so a consumer who follows it will get Keycloak provisioned but the provider may never be advertised to the web client. Please make the documented path a single, complete toggle.

Related: replicated/openhands.yaml wires the new lowercase enterpriseSso block and ENABLE_ENTERPRISE_SSO, but never sets the capital enterpriseSSO, so OH_WEB_CLIENT_PROVIDERS_CONFIGURED won't include enterprise_sso on Replicated installs.

Question - ENABLE_ENTERPRISE_SSO vs OH_WEB_CLIENT_PROVIDERS_CONFIGURED

The button appears to be driven by ENABLE_ENTERPRISE_SSO (presence-checked, set only in the Replicated layer), while OH_WEB_CLIENT_PROVIDERS_CONFIGURED is driven by the capital enterpriseSSO.enabled. Are both needed? Note that for pure-Helm consumers nothing sets ENABLE_ENTERPRISE_SSO, so it would help to clarify which mechanism is authoritative and ensure the raw chart covers it.

Looks good (no action needed)

• Failure-safety is correct: the provisioning runs in a subshell terminated by || echo "WARNING...", so an unreachable/invalid metadata URL warns without blocking pod startup, and the internal exit 1 only exits the subshell.
• validateSignatures: "true", trustEmail documented, and hideOnLogin: true (valid as a top-level field on the bundled Keycloak - bitnami chart 24.7.5) are all sensible defaults.
• The upsert style (existence check -> PUT/POST, keycloak_api_call) matches the existing realm/IdP/mapper loop vocabulary, and a separate block is justified since the SAML metadata is imported dynamically and can't live in the static realm template.
• The Replicated env mapping that renders true vs an empty string (rather than the literal false) correctly matches the app's presence-check semantics and mirrors the existing conditional-literal pattern in this file.

This review was generated by an AI agent (OpenHands) on behalf of the user.

Reviewed at 349b9243.

Verified locally before writing this:

• helm unittest on the new enterprise_sso_test.yaml plus the changed values_schema_test.yaml: 13 passed.
• pytest scripts/test_keycloak_realm_template.py: 17 passed, 1 pre-existing failure (test_keycloak_identity_provider_socket_timeout, needs built subcharts, unrelated to this PR).
• Rendered keycloak-config.sh for both the auto-config and the disable branch and ran sh -n and bash -n on each. Both clean.
• Ran the extracted identity-provider jq filter through real jq against a metadata import response with no signing certificate.
• Confirmed the bundled Keycloak is 26.3.0 (bitnami chart 24.7.5), so hideOnLogin as a top-level field is correct for this version.

Nice work on passing the operator inputs as container env and encoding them with jq --arg. That is the right call, and the jq-filter test is the one new test I would keep as is.

Three things I think are worth changing before this merges.

1. ENABLE_ENTERPRISE_SSO is only set on the Replicated path

replicated/openhands.yaml is the only place it is set. charts/openhands/templates/_env.yaml adds enterprise_sso to OH_WEB_CLIENT_PROVIDERS_CONFIGURED, but nothing sets ENABLE_ENTERPRISE_SSO. The enterprise server also builds its own providers_configured list from ENABLE_ENTERPRISE_SSO, and that path does not read the OH_WEB_CLIENT_* form. So a plain Helm install that sets enterpriseSSO.enabled: true can get the identity provider created in Keycloak and still show no login button.

The chart's existing convention for this lives in _env.yaml: ENABLE_JIRA, ENABLE_JIRA_DC, and ENABLE_LINEAR are all {{- if .Values.<x>.enabled }} blocks with value: "true". Following that here fixes the self-hosted case and lets you drop the repl{{ if }}...repl{{ end }} form.

One related detail: the current form renders value: "" when the toggle is off, not an absent variable. I confirmed the env dedup wrapper emits the empty entry. It is harmless today because the app strips the value before testing it, but the comment's "presence-checked" wording does not match that.

2. validateSignature is forced to "true" even with no signing certificate

The filter ends with config: (. + {"syncMode": "IMPORT", "validateSignature": "true"}), and the import guard only checks idpEntityId and singleSignOnServiceUrl. Running the filter against a certificate-less import response gives:

"config": {
  "idpEntityId": "https://idp/e",
  "singleSignOnServiceUrl": "https://idp/sso",
  "postBindingResponse": "true",
  "validateSignature": "true",
  "syncMode": "IMPORT"
}

No signingCertificate. Keycloak sets validateSignature itself based on what it finds in the descriptor, and this overrides that to true with nothing to validate against. The init container then logs enterprise_sso SAML identity provider configured. and the pod goes Ready, but every login fails at signature verification. Adding has("signingCertificate") to the guard would catch it.

Worth considering instead: set useMetadataDescriptorUrl: "true" and metadataDescriptorUrl and let Keycloak fetch the descriptor itself. That removes the import call and its response validation entirely. It also fixes a second problem. Today the certificate is pinned at pod-start time, so an identity provider certificate rotation breaks SSO until someone restarts the pod.

3. No guard for a metadata URL with no chart-provisioned Keycloak

keycloak-config-script.yaml renders only when openhands.keycloakProvisionRealm is true, which needs keycloak.enabled or keycloak.provisionRealm. With both false and an external Keycloak, idpMetadataUrl passes the schema and silently does nothing. templates/validations.yaml is the established place for this, and the guard is a couple of lines.

Smaller points

The disable path keys off a retained idpMetadataUrl. On Replicated that works, since KOTS keeps a hidden item's stored value, and there is already a comment in replicated/openhands.yaml saying so. For a chart consumer the natural action is to set enabled: false and delete the URL, which skips the branch. The provider stays enabled and kc_idp_hint=enterprise_sso keeps working, which is the bypass the branch exists to prevent. The README asks operators to remember to retain the URL for one rollout, and that is easy to get wrong. What is the reason for gating on idpMetadataUrl rather than reconciling whenever enterpriseSSO.enabled is false? That needs no marker value at all.

Failures are invisible, and the reused token makes them more likely. The subshell swallows everything, the pod goes Ready, and the only signal is one init-container log line. The block also reuses the ACCESS_TOKEN minted before the whole realm sync of clients, identity providers, and mappers. Master admin-cli tokens are short-lived, so a slow realm update turns this into a 401 that fails the import guard, and the result is a healthy-looking deploy with no identity provider. Re-minting the token at the top of the block is cheap. The disable branch has the same issue and is quieter still: a 401 body has no internalId, so it takes the "does not exist" path and prints nothing.

trustEmail: true is hardcoded. That means the SAML assertion's email is trusted for auto-linking to existing accounts, including accounts created through GitHub OAuth, with no verification step. That is a reasonable default for corporate SSO, but it is a trust decision made on the operator's behalf and it is not mentioned in the README or the installer help text.

The "Company SSO" fallback lives in the jq filter while values.yaml has displayName: "". Putting the default in values.yaml, or using | default "Company SSO" in deployment.yaml, makes it visible to chart consumers and gives one source of truth. replicated/config.yaml could then just carry default: "Company SSO".

Some of the new tests assert the diff back to itself. The exact values.yaml key order and formatting, "enterpriseSso" not in ..., "validateSignatures" not in ..., and the exact regex string in config.yaml all guard typos that were never on main, and they break on a harmless reformat. Meanwhile nothing asserts that the ENTERPRISE_SSO_* env vars land on the keycloak-config init container. The pytest test greps template text, and the helm-unittest suite only checks containers[0]. One assert on initContainers[?(@.name=="keycloak-config")].env would catch a real regression, and a certificate-less metadata case for the jq filter would be worth more than the string greps.

Two tiny ones. IMPORT_REQUEST sends alias, which import-config ignores. And the values.schema.json pattern ^$|^https:// is looser than the installer regex, since it accepts a bare https:// and embedded whitespace.

This comment was drafted by an AI agent on behalf of the user.

🔴 Needs improvement — the overall design is reasonable, but three important failure/operability gaps need attention.

Current status: The inline findings were revalidated on head a99f064; the merge from main did not change the Enterprise SSO implementation.

[CRITICAL ISSUES]

• The best-effort shell wrapper can suppress failed Keycloak operations and report success.
• Init-container Enterprise SSO variables bypass the chart's explicit environment de-duplication/override contract.
• The documented manual setup path omits the mapper this change says OpenHands relies on.

[RISK ASSESSMENT]

• [Overall PR] ⚠️ Risk Assessment: 🟡 MEDIUM
  Defaults are inert and CI is green, but this changes authentication provisioning. The identified paths can leave SSO unconfigured or partially configured while deployment appears healthy.

VERDICT:
❌ Needs rework: Address the three inline issues before merging.

KEY INSIGHT:
Authentication provisioning must fail observably and preserve the chart's established override contract, even when startup remains best-effort.

This review was generated by an AI agent (OpenHands) on behalf of the user.

Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:

1. Add a .agents/skills/custom-codereview-guide.md file to your branch (or edit it if one already exists) with the /codereview trigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.
2. Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
3. When your PR is merged, the guideline file goes through normal code review by repository maintainers.

Resolve with AI? Install the iterate skill in your agent and run /iterate to automatically drive this PR through CI, review, and QA until it's merge-ready.

Was this review helpful? React with 👍 or 👎 to give feedback.

Reviewed at c1ba37d against the OpenHands-Cloud Helm chart guidelines. All 22 checks are green and I re-verified the shell/jq behaviour locally rather than reading it off the diff.

The overall shape is good: one canonical enterpriseSSO values key, HTTPS enforced in both the JSON schema and the KOTS regex, operator input passed as container env and encoded with jq --arg (I confirmed a display name of Company "Platform" $(touch /tmp/should-not-run) round-trips as data), inert defaults, and a render guard that fails fast when idpMetadataUrl is set without chart-managed realm provisioning. The earlier review rounds clearly moved this a long way.

Two important items remain. Neither blocks the design, but both are the kind of thing that turns into a support ticket rather than a failed deploy.

[IMPORTANT]

1. keycloak_api_call still reports success for an empty response body, so the SSO write paths can log "Created identity provider: enterprise_sso" when nothing was written. Verified locally: keycloak_api_call "printf ''" returns 0. Because every call uses curl -s without -f or %{http_code}, HTTP status is discarded, and a PUT that succeeds (204, empty body) is indistinguishable from one that failed with an empty body. The HTML-error-page case is now caught by the new parse guard — good — so this is a narrow but real residual gap. keycloak_get_optional, added in this same PR, already has the right shape; routing the four SSO POST/PUT calls through a status-checking helper would close it cheaply and consistently. This matters more here than elsewhere in the script because the SSO block is deliberately best-effort: a masked failure produces a green deploy plus a login button that dead-ends at Keycloak, with no signal anywhere except init-container logs.

2. idpMetadataUrl is doing two jobs: it supplies the metadata and marks the provider as chart-managed. The disable branch keys off .Values.enterpriseSSO.idpMetadataUrl alone, so the README has to instruct operators to keep a metadata URL populated while turning SSO off — a value whose name promises "where to fetch metadata" now silently means "the chart owns this provider." That is the one knob, one effect rule in the repo guidelines, and the effect isn't predictable from the key name. It also has a concrete failure mode: if an operator clears the URL in the same rollout that flips the toggle off (the intuitive action, and what the KOTS UI nudges toward since the field is hidden by when:), neither branch renders and the provider stays enabled in Keycloak — reachable via a direct kc_idp_hint=enterprise_sso request, which is exactly what the disable path exists to prevent. An explicit ownership marker (e.g. a managed-by entry written into the provider's config at create time, or a separate values key) would make the intent legible and survive the URL being cleared. Related question I could not settle from the repo alone: does ConfigOption "enterprise_sso_idp_metadata_url" still return the stored value once when: hides the field? If KOTS returns empty for hidden items, the disable path never renders in Replicated installs — the primary target of this PR. Worth confirming before merge.

[MINOR]

3. "Company SSO" is defaulted in three places (values.yaml, the | default in deployment.yaml, and replicated/config.yaml). Since values.yaml already sets it, the template fallback only fires when someone explicitly sets it to empty. Dropping the | default would leave one authoritative chart default.

4. The two env entries in deployment.yaml re-implement the .Values.env precedence contract with hasKey guards instead of flowing through openhands.env. It renders correctly today — I confirmed the override test exercises the real helm template output — but it is a second copy of a cross-cutting rule that will drift from the helper. If keeping them out of openhands.env.defaults is deliberate (to avoid leaking SSO env into the app and litellm containers), a one-line comment saying so would help the next person.

[RISK] Moderate but well-contained. Defaults are inert, so existing installs and OSS values render unchanged, and the render guard prevents the half-configured combination. The residual exposure is operational rather than structural: a provisioning failure is best-effort by design and, given item 1, may not be visible at all; and item 2's disable path can silently no-op. Both surface as "SSO is broken/still on" rather than a failed rollout, which is the harder class to debug.

Happy to approve once items 1 and 2 are addressed or explicitly dismissed with reasoning.

This review was generated by an AI agent (OpenHands) on behalf of the user.

Reviewed at 7c02209 against the OpenHands-Cloud Helm chart guidelines. I rendered the chart and executed the generated script against a stub Keycloak rather than reading behaviour off the diff.

What I verified locally

• helm unittest on enterprise_sso_test.yaml, validations_test.yaml, values_schema_test.yaml: 31 passed.
• pytest scripts/test_keycloak_realm_template.py: 24 passed.
• Rendered keycloak-config.sh in automatic, manual, and disabled modes; all pass sh -n and bash -n.
• Ran the rendered enterprise-SSO block end to end against a stub Keycloak. It POSTs the provider and the mapper, and the stored provider carries validateSignature=true, syncMode=IMPORT, openhandsManaged=true, and attribute.value=enterprise_sso:saml. Re-ran the toggle-off block against that same stub state and it correctly flipped enabled to false.

The earlier round of feedback was genuinely addressed: transport and HTTP status failures now propagate, openhandsManaged replaced the overloaded idpMetadataUrl ownership inference, and the manual mapper contract is documented in both the README and the installer help text. The Keycloak provisioning logic is correct on the happy path.

Two findings below are worth attention before merge. Neither is a blocker for the provisioning logic itself; both are reachable states an operator can land in through the installer UI.

Important — toggling Enterprise SSO off in the installer renders an un-deployable chart.

validations.yaml fails on idpMetadataUrl being non-empty regardless of enterpriseSSO.enabled. But replicated/config.yaml marks enterprise_sso_idp_metadata_url with when: enterprise_sso_enabled == "1", and hiding a KOTS config item does not clear the value the operator already typed. So the natural "turn Enterprise SSO back off" path leaves enabled: false with a stale URL still set.

That combination is fine while keycloak.enabled=true (the Replicated default), but it fails to render on any split-Keycloak deployment. Verified:

$ helm template t charts/openhands --set enabled=true \
    --set keycloak.enabled=false --set keycloak.provisionRealm=false \
    --set enterpriseSSO.enabled=false \
    --set-string enterpriseSSO.idpMetadataUrl=https://idp.example.com/saml/metadata
Error: enterpriseSSO.idpMetadataUrl requires chart-managed Keycloak realm provisioning...

The operator disabled the feature and the deploy now fails on a value they cannot see in the UI. Guarding on the URL only when the feature is on — and .Values.enabled .Values.enterpriseSSO.enabled .Values.enterpriseSSO.idpMetadataUrl (not (include ...)) — matches the intent ("you asked for auto-provisioning but there is no realm to provision into") and makes the toggle-off path inert, which is what the disable branch already assumes.

Important — the disable path is one-way; re-enabling never restores the provider.

The toggle-off branch only sets .enabled = false on the managed provider, which is the right conservative choice. But the re-enable path is and enabled idpMetadataUrl, and per the finding above the metadata URL is the value most likely to be missing after a toggle-off/toggle-on cycle. In that state:

• enabled: true, no URL → ENABLE_ENTERPRISE_SSO is set and enterprise_sso is advertised in OH_WEB_CLIENT_PROVIDERS_CONFIGURED,
• but neither branch of the script renders, so the disabled provider in Keycloak stays disabled.

I confirmed both halves: rendering with enabled=true, idpMetadataUrl="" emits the login-provider env vars but produces no provisioning and no reconciliation block, and my stub still held enabled: false after the disable run. The result is a login button that leads to a disabled IdP, with no message anywhere explaining why. Re-enabling a provider that already carries openhandsManaged=true is the symmetric counterpart to the disable branch and closes the loop.

The remaining notes are minor and I would not hold the PR for them.

This review was generated by an AI agent (OpenHands) on behalf of the user.